Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ROS-independent CI Builds #818

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marip8
Copy link
Member

@marip8 marip8 commented Mar 2, 2023

Addresses #817 to allow CI builds that do not source /opt/ros/<distro> during the build.

Here is an example CI run where this capability was used

@marip8 marip8 force-pushed the update/ros-independent-ci branch from 95af2d7 to 56dca0f Compare March 2, 2023 21:50
@mathias-luedtke
Copy link
Member

IMHO this is out of scope of industrial_ci.
The scripts assume a ROS environment to function properly.

Copy link
Member

@mathias-luedtke mathias-luedtke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to sacrifice the sanity checks for an edge case, which is not really supported..

@@ -172,7 +172,7 @@ function run_source_tests {

ici_step "setup_rosdep" ici_setup_rosdep

extend=${UNDERLAY:?}
extend=${UNDERLAY:-}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This knocks out one of the sanity tests.

@@ -406,7 +406,7 @@ function ici_source_setup {
local extend=$1; shift
if [ ! -f "$extend/setup.bash" ]; then
if [ "$extend" != "/opt/ros/$ROS_DISTRO" ]; then
ici_error "'$extend' is not a devel/install space"
ici_log "'$extend' is not a devel/install space; skipping source"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this is an important sanity check to test against misspelled parameters.

Comment on lines +91 to +94
export BUILDER=${BUILDER:-colcon}
export ROS_VERSION=0
export ROS_VERSION_EOL=false
export ROS_PYTHON_VERSION=${ROS_PYTHON_VERSION:-3}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are questionable defaults.

Copy link
Member Author

@marip8 marip8 Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your suggested alternatives? Python2 is EOL, so Python3 seems reasonable. colcon is the only builder I am familar with that can build code such that a downstream ROS1 or ROS2 workspace can use it. The ROS version cannot be EOL since this is for a ROS-independent build. ROS_VERSION is required by other parts of ICI, so anything < 1 seems okay

@marip8
Copy link
Member Author

marip8 commented Mar 10, 2023

IMHO this is out of scope of industrial_ci.

Maybe at the moment, but I'm proposing this PR to support these types of builds as a feature of industrial_ci rather than a convenient hack.

At least for ROS-I Americas, I think ROS-independent builds are going to become more common as we separate the core functionality of our tools from the ROS interfaces that make them convenient to use. As stated in #817, I personally already have several ROS-I related projects that could leverage this type of build today, with more planned in the near future. We could certainly gather feedback from others if there is a question about general interest in this capability as a feature. In my opinion, there is a ton of value in the ICI pipeline: nested builds, docker image creation, testing, dependency installation, etc. I would rather not re-implement this whole pipeline elsewhere when it only requires very minor modifications to support the builds I am interested in.

Regarding the sanity checks, I understand the concern. What would be an appropriate way to keep them while being able to keep these added features (i.e., no UNDERLAY required and no sourcing /opt/ros/$ROS_DISTRO)? An additional variable(s ) that must be explicitly set to enable a ROS-independent build?

@mathias-luedtke
Copy link
Member

What is so bad about sourcing /opt/ros?
In your example (marip8/opw_kinematics#1) you even install the ROS repositories manually..

@mathias-luedtke
Copy link
Member

#834 should be sufficient, it breaks ros_prerelease tests, but I will have to come up with a better implementation for those anyway

@marip8
Copy link
Member Author

marip8 commented Jun 2, 2023

What is so bad about sourcing /opt/ros?
In your example (marip8/opw_kinematics#1) you even install the ROS repositories manually..

Mostly because I want to do builds where ROS isn't actually installed, so there is no /opt/ros/... to source. In the opw_kinematics example, I don't actually install ROS. I'm just using some ROS ecosystem tools like colcon to build and rosdep to install dependencies that are distributed on Ubuntu (i.e. from rosdep/base.yaml). I still have to add the ROS source list and keys in order to install these tools, but ROS itself was never installed.

@Ryanf55
Copy link

Ryanf55 commented Feb 14, 2024

The whole point of colcon, ament, and vcs is they are separate from ROS. Perhaps an alternative method is to contribute the actions directly to those repositories?IE - VCS has a clone action, and colcon has a build action and a test action?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants